-
Notifications
You must be signed in to change notification settings - Fork 48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: TypeError: headers is not iterable #210
Conversation
At runtime headers may not be an `instanceof` Headers. This causes a lot of bugs. the function may get a js object as an argument. Instead of looping directly through `headers`, I get the entries then loop through them.
Hi @mjad218 Thank you for the PR. Can you run |
Sure, will push a commit soon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Thanks! Merging. |
Hi, @mjad218, Thanks for creating the pull request. I'm sorry, but I didn't understand the need for this change. At least inside "node-server," I don't think anything other than "Headers" is passed as an argument. Does this mean it is used by directly importing it from the application? I'd like to know the specific use case. Also, when merging, we may need to add test code. |
@usualoma |
This is the adapter I am using |
I believe what you are saying is correct given that it is called with valid arguments and you validate the headers object. I reviewed its usage in the package here, I noticed whenever it's used, it has an There was one place in the package that did not have this condition. line 90 Moving the validation inside the function itself is good; at runtime, you don't know what type of arguments it will get. (TS does not guarantee runtime checks) |
@mjad218 Thanks for the reply! In the following line, we are referencing the headers property of the Line 90 in e63a808
https://developer.mozilla.org/en-US/docs/Web/API/Response/headers It is possible to break this convention in an application, but that is its responsibility, and it is not something that the node-server should handle. Also, I don't think any Hono users have written code that returns a I think that I don't need these changes. |
@usualoma I am using Nest.js and trying to get it working with Hono. And it seems that I spent the past few days walking through Hono source code and I like how it's written and structured.
I think this is the case, Nest.js is not designed to work with Hono. I am just trying to build an adapter for it. It works best with Express and Fastify. |
@mjad218 |
Great, Glad to hear that |
Sure, that's fine |
At runtime headers may not be an
instanceof
Headers.This causes a lot of bugs.
the function may get a js object as an argument.
Instead of looping directly through
headers
, I get the entries then loop through them.